Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use placeholder text for Provider Host input #8754

Merged
merged 1 commit into from
Mar 11, 2022
Merged

Conversation

andrew-farries
Copy link
Contributor

Description

Change the default value in the Provider Host field on the new integration modal from a value that the user must delete before entering a valid host, to a placeholder that disappears when the input field has focus.

Before:

Screen.Recording.2022-03-10.at.14.48.44.mov

After:

Screen.Recording.2022-03-10.at.14.13.36.mov

Related Issue(s)

How to test

Open the modal at Settings -> Integrations -> Add integration and check that the value in the Provider Host field is now a placeholder not a value that must be deleted.

Release Notes

NONE

Documentation

No changes needed.

Previously, this used a value in the input text field that had to be
deleted by the user before the user could enter a real value.
@andrew-farries andrew-farries requested a review from a team March 11, 2022 12:55
@github-actions github-actions bot added team: webapp Issue belongs to the WebApp team and removed size/S labels Mar 11, 2022
Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and works as advertised. Thanks! 🙌

One small nit is that the callback URL no longer updates according to the placeholder value, but since you do need to input an actual hostname, that's actually okay. 👍

@roboquat roboquat merged commit 0a62489 into main Mar 11, 2022
@codecov
Copy link

codecov bot commented Mar 11, 2022

Codecov Report

Merging #8754 (d3b36c1) into main (a9715ee) will decrease coverage by 1.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #8754      +/-   ##
==========================================
- Coverage   12.31%   11.17%   -1.14%     
==========================================
  Files          20       18       -2     
  Lines        1161      993     -168     
==========================================
- Hits          143      111      -32     
+ Misses       1014      880     -134     
+ Partials        4        2       -2     
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@roboquat roboquat deleted the af/input-placeholder branch March 11, 2022 13:02
@gtsiolis
Copy link
Contributor

gtsiolis commented Mar 11, 2022

One small nit is that the callback URL no longer updates according to the placeholder value, but since you do need to input an actual hostname, that's actually okay.

Nice catch, @jankeromnes! We could restore the example URL when deleting the input or use ... while the input is empty.

Thanks for making this change @andrew-farries!

@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants